feat: support S3-compatible storage providers (s3Url, s3ForcePathStyle, caCert)#75
Conversation
Add environment variable constants and UploaderConfig struct fields for S3-compatible storage provider support: s3Url, s3ForcePathStyle, insecureSkipTLSVerify, and caCert. These will be wired through the config pipeline in subsequent commits. Refs: migtools#23 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add S3-compatible storage fields to DatamoverPodConfig and wire them as environment variables in the datamover pod. Update LoadConfigFromEnv() to parse the new env vars, including boolean parsing for s3ForcePathStyle and insecureSkipTLSVerify. Refs: migtools#23 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Read s3Url, s3ForcePathStyle, insecureSkipTLSVerify, and caCert from bsl.Spec.Config in extractBSLConfig(). Propagate the new fields through buildDatamoverPodConfig() and initObjectStoreFromBSL() so both the datamover pod and controller-side object store receive the full config. Refs: migtools#23 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Change NewS3ObjectStore from individual parameters to a config map, removing the intermediate map construction. Simplify InitObjectStore by passing credentialsData and S3-compatible settings through the map, letting Init() handle temp file creation in one place. Refs: migtools#23 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
… S3 client Wire the S3-compatible BSL config keys into the S3 client creation: - s3Url sets BaseEndpoint for custom S3 endpoints (MinIO, NooBaa, Ceph) - s3ForcePathStyle enables UsePathStyle addressing - caCert configures custom CA certificate for private endpoints - insecureSkipTLSVerify skips TLS certificate verification URL scheme is validated (http/https only) and invalid CA PEM is rejected with a clear error. TLS configuration follows the same approach as Velero's AWS plugin. Fixes: migtools#23 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Add tests for coverage gaps identified during review:
- InitObjectStore with S3 settings (configMap building)
- InitObjectStore with unknown provider (S3-compatible fallback)
- Init with insecureSkipTLSVerify alone
- Boolean parsing edge cases ("false", "yes" must not parse as true)
- strconv.FormatBool roundtrip (controller → env var → uploader)
Refs: migtools#23
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Use http.DefaultTransport.Clone() instead of a bare http.Transport{}
when building the custom TLS client. This preserves Go's default
connection pooling, timeouts, keep-alive, and proxy settings.
Refs: migtools#23
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Refs: migtools#23 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR extends S3 object-store support to handle S3-compatible storage providers (MinIO, NooBaa, Ceph RGW) by routing additional BackupStorageLocation configuration settings through the datamover pod and into object-store initialization. Configuration for custom endpoints, path-style addressing, and custom CA certificates for TLS now flows from the controller through environment variables to the uploader, enabling S3-compatible backends with proper endpoint and security handling. ChangesS3-Compatible Configuration Plumbing
Sequence DiagramsequenceDiagram
participant BSL as BackupStorageLocation
participant Controller as extractBSLConfig
participant Pod as buildDatamoverPod
participant Env as LoadConfigFromEnv
participant Store as S3ObjectStore.Init
BSL->>Controller: spec.Config (s3Url, s3ForcePathStyle, caCert, insecureSkipTLSVerify)
Controller->>Pod: bslConfig fields
Pod->>Env: environment variables (BSLS3URL, etc.)
Env->>Store: UploaderConfig (BSLS3URL, BSLS3ForcePathStyle, BSLCACert, BSLInsecureSkipTLSVerify)
Store->>Store: parseS3URL, buildTLSHTTPClient
Store->>Store: configureBaseEndpoint, enablePathStyle, attachCustomCA
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/cherry-pick oadp-1.6 |
|
@shubham-pampattiwar: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/uploader/run_test.go (1)
140-384:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReduce
TestLoadConfigFromEnvcyclomatic complexity to unblock lintLine 140 currently fails gocyclo (31 > 30), so this PR cannot pass CI as-is. Please split this test into smaller focused helpers/subtests.
Suggested split
func TestLoadConfigFromEnv(t *testing.T) { - // all cases + all validations in one function + t.Run("core required/default config", testLoadConfigFromEnvCore) + t.Run("S3-compatible config parsing", testLoadConfigFromEnvS3) } + +func testLoadConfigFromEnvCore(t *testing.T) { + // existing non-S3 cases +} + +func testLoadConfigFromEnvS3(t *testing.T) { + // existing S3 URL / forcePath / insecure / caCert cases +}As per coding guidelines, "After editing Go controller or other source files, run
make lint-fixto auto-fix code style andmake testto run unit tests."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/uploader/run_test.go` around lines 140 - 384, The TestLoadConfigFromEnv function is too complex (gocyclo > 30); refactor by extracting repeated setup and assertion logic into small helper functions and per-case subtests: create helpers like setEnvVars(map[string]string) to apply/clear environment, runLoadConfigExpect(t, name, envVars, expectError, validate) to call LoadConfigFromEnv and assert error/no-error, and smaller validators (e.g., validateDefaults, validateS3Settings) to hold the validation blocks; then rewrite TestLoadConfigFromEnv to iterate test cases but call those helpers (referencing TestLoadConfigFromEnv, LoadConfigFromEnv, and the validate closures) so the main test body is short and each subtest focuses on one concern, reducing cyclomatic complexity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/pod_builder_test.go`:
- Around line 347-424: The new test TestS3CompatibleBooleanRoundtrip contains
gofmt/formatting issues; run the formatter (or run make lint-fix) and reformat
this test block so it passes CI; ensure spacing/indentation in the
TestS3CompatibleBooleanRoundtrip function and its use of buildDatamoverPod,
container.Env loop, envMap lookups, and strings.EqualFold parsing (and
references like uploader.EnvBSLS3ForcePathStyle /
uploader.EnvBSLInsecureSkipTLSVerify) are properly formatted according to gofmt
before committing.
---
Outside diff comments:
In `@pkg/uploader/run_test.go`:
- Around line 140-384: The TestLoadConfigFromEnv function is too complex
(gocyclo > 30); refactor by extracting repeated setup and assertion logic into
small helper functions and per-case subtests: create helpers like
setEnvVars(map[string]string) to apply/clear environment, runLoadConfigExpect(t,
name, envVars, expectError, validate) to call LoadConfigFromEnv and assert
error/no-error, and smaller validators (e.g., validateDefaults,
validateS3Settings) to hold the validation blocks; then rewrite
TestLoadConfigFromEnv to iterate test cases but call those helpers (referencing
TestLoadConfigFromEnv, LoadConfigFromEnv, and the validate closures) so the main
test body is short and each subtest focuses on one concern, reducing cyclomatic
complexity.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cb325027-6f49-41cb-b292-5afecfd5ab1d
📒 Files selected for processing (9)
internal/controller/kubevirt_dataupload_controller.gointernal/controller/kubevirt_dataupload_controller_test.gointernal/controller/pod_builder.gointernal/controller/pod_builder_test.gopkg/uploader/objectstore.gopkg/uploader/objectstore_test.gopkg/uploader/run.gopkg/uploader/run_test.gopkg/uploader/types.go
Run gofmt on files with alignment issues and add nolint:gocyclo directive to TestLoadConfigFromEnv (complexity 31, threshold 30) matching the existing pattern in TestBuildDatamoverPod. Refs: migtools#23 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: shubham-pampattiwar, sseago, weshayutin The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@shubham-pampattiwar: new pull request created: #76 DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
4 participants
Summary
Adds support for S3-compatible storage providers (MinIO, NooBaa, Ceph RGW) by wiring four BSL config keys through the full configuration pipeline:
s3Url— custom S3 endpoint URL (setsBaseEndpointon the S3 client)s3ForcePathStyle— path-style addressing (setsUsePathStyle, required by most S3-compatible stores)caCert— PEM-encoded custom CA certificate for private endpointsinsecureSkipTLSVerify— skip TLS certificate verificationThese keys match the ones supported by Velero's AWS plugin.
Changes
The config flows through four layers, each touched by this PR:
kubevirt_dataupload_controller.go) —extractBSLConfig()reads the new keys frombsl.Spec.Configpod_builder.go) — passes values as env vars to the datamover podrun.go,types.go) —LoadConfigFromEnv()parses the env varsobjectstore.go) —Init()applies endpoint URL, path-style, and TLS settingsAdditionally:
NewS3ObjectStore()to accept a config map instead of individual parametersInitObjectStore()by delegating credential temp-file handling toInit()http.DefaultTransportto preserve connection pool settingsBackwards Compatibility
All new fields default to zero values. Existing BSL configs without the new keys work exactly as before.
Test Plan
extractBSLConfig()— S3 key extraction, case-insensitive booleans, defaultsLoadConfigFromEnv()— parsing, defaults, boolean edge cases ("false", "yes")buildDatamoverPod()— env vars set correctly, boolean roundtripInit()— s3Url, forcePathStyle, caCert, insecureSkipTLSVerify, invalid inputsInitObjectStore()— S3 settings passthrough, unknown provider fallbackisValidS3URLScheme()andbuildTLSHTTPClient()s3Url+s3ForcePathStyle+insecureSkipTLSVerify, backup completed successfully with qcow2 uploaded to MinIOFixes: #23
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests